Re: [RFC PATCH 05/16] hw/arm/smmuv3: Add page table walk for stage-2

[Eric Auger]

On 2/16/23 14:09, Mostafa Saleh wrote:
> Hi Eric,
>
> On Wed, Feb 15, 2023 at 05:52:39PM +0100, Eric Auger wrote:
>>> In preparation for adding stage-2 support. Add Stage-2 PTW code.
>>> Only Aarch64 fromat is supported as stage-1.
>> format
> I will update it.
>
>>> +        uint64_t subpage_size = 1ULL << level_shift(level, granule_sz);
>>> +        uint64_t mask = subpage_size - 1;
>>> +        uint32_t offset = iova_level_offset(iova, inputsize, level, 
>>> granule_sz);
>>> +        uint64_t pte, gpa;
>>> +        dma_addr_t pte_addr = baseaddr + offset * sizeof(pte);
>>> +        uint8_t ap;
>>> +
>>> +        if (get_pte(baseaddr, offset, &pte, info)) {
>>> +                goto error;
>>> +        }
>>> +        trace_smmu_ptw_level(level, iova, subpage_size,
>>> +                             baseaddr, offset, pte);
>> I can the trace point names should be updated as well (and
>> differentiated between S1/S2)
> I was thinking we could leave those with stage argument, and only
> update trace_smmu_ptw_level to have stage argument as the others.
yes as far as the stage is properly populated that's fine.
>
>>> +        if (is_permission_fault_s2(ap, perm)) {
>>> +            info->type = SMMU_PTW_ERR_PERMISSION;
>> don't we have to different S1 versus S2 faults?
> Yes, I missed that, I see setting info->u.f_walk_eabt.s2 should be
> enough, this will set the S2 field in the fault event.
>
>>>  int smmu_ptw(SMMUTransCfg *cfg, dma_addr_t iova, IOMMUAccessFlags perm,
>>>               SMMUTLBEntry *tlbe, SMMUPTWEventInfo *info)
>>>  {
>>> -    if (!cfg->aa64) {
>>> -        /*
>>> -         * This code path is not entered as we check this while decoding
>>> -         * the configuration data in the derived SMMU model.
>>> -         */
>>> -        g_assert_not_reached();
>> if that's still true for S2, maybe keep that check here upfront?
> Stage-2 is checked in STE parsing and throws BAD_STE if not aa64,
> which I believe is not correct, however I think we can just call
> g_assert_not_reached() during STE parsing, I don’t see added value for
> saving this field in SMMUTransCfg if we don’t use it.
I agree. I guess we provisionned for this field in the prospect of
completing the emulation but I don't think we care.
> I am not sure why this check exists for stage-1 as it is hardcoded in
> decode_cd anyway.
>
>>> +{
>>> +    uint64_t ret;
>>> +    /*
>>> +     * Get the number of bits handled by next levels, then any extra bits 
>>> in
>>> +     * the address should index the concatenated tables. This relation can
>>> +     * deduced from tables in ARM ARM: D8.2.7-9
>>> +     */
>>> +    int shift = (SMMU_MAX_LEVELS - start_level) * (granule - 3) + granule;
>> can't we factorize anything with the S1 PTW?
>> indexmask = (1ULL << (inputsize - (stride * (4 - level)))) - 1;
> Yes, I think we can refactor some of these in common functions/macros, I
> will do that in v2.
I guess that's a trade-off between beeing close enough to the spec and
maybe its pseudo-code and having both S1/S2 codes looking similar.

Eric
>
>
>>> @@ -28,6 +28,7 @@
>>>  #define SMMU_PCI_DEVFN(sid)   (sid & 0xFF)
>>>  
>>>  #define SMMU_MAX_VA_BITS      48
>>> +#define SMMU_MAX_LEVELS       4
>> can't this be reused as well with S1 PTW?
> I believe yes, I will update it.
>
> Thanks,
> Mostafa
>