Re: [Qemu-devel] [PATCH v9 03/14] hw/arm/smmu-common: VMSAv8-64 page table walk

[Auger Eric <eric.auger@redhat.com>]

Hi Peter,
On 07/03/18 17:35, Peter Maydell wrote:
> On 7 March 2018 at 16:23, Auger Eric <eric.auger@redhat.com> wrote:
>> Hi Peter,
>>
>> On 06/03/18 20:43, Peter Maydell wrote:
>>> On 17 February 2018 at 18:46, Eric Auger <eric.auger@redhat.com> wrote:
> 
>>>> +int smmu_ptw(SMMUTransCfg *cfg, dma_addr_t iova, IOMMUAccessFlags perm,
>>>> +             IOMMUTLBEntry *tlbe, SMMUPTWEventInfo *info)
>>>> +{
>>>> +    if (!cfg->aa64) {
>>>> +        error_setg(&error_fatal,
>>>> +                   "SMMUv3 model does not support VMSAv8-32 page walk yet");
>>>
>>> This sort of guest-provokable error should not be fatal -- log
>>> it with LOG_UNIMP and carry on as best you can (in this case
>>> the spec should say what happens for SMMUv3 implementations which
>>> don't support AArch32 tables).
>> This code should never been entered as the check is done when decoding
>> the CD in the smmuv3. However since it is a base class I wanted to
>> enphase the ptw was only supporting AArch32.
> 
> Ah, right. That should be an assert() with a brief comment
> about why the condition will never trigger.
> 
> 
>>>> +#define is_permission_fault(ap, perm) \
>>>> +    (((perm) & IOMMU_WO) && ((ap) & 0x2))
>>>
>>> Don't we also need to check AP bit 1 in some cases?
>>> (when the StreamWorld is S or NS EL1 and either (a) the incoming
>>> transaction has its attrs.user = 1 and STE.PRIVCFG is 0b0x, or
>>> (b) STE.PRIVCFG is 0b10).
>> I think I don't need to as I don't support this feature at the moment:
>> spec says:
>> "When SMMU_IDR1.ATTR_PERMS_OVR=0, this field is RES0 and the incoming
>> PRIV attribute is used."
>> But to be honest I was not aware this existed ;()
> 
> I think you still need to check the incoming transaction
> for user vs priv, even if you don't support STE.PRIVCFG.

On the CPU side, you have MemTxAttrs as input from get_phys_addr_lpae().

On IOMMU side, the current input callback for translation is

static IOMMUTLBEntry smmuv3_translate(IOMMUMemoryRegion *mr, hwaddr
addr, IOMMUAccessFlags flag)

where IOMMUAccessFlags just is R/W access flag.

So I am not sure I have acess to those user/priv attributes.
> 
>>>> +
>>>> +#define PTE_AP_TO_PERM(ap) \
>>>> +    (IOMMU_ACCESS_FLAG(true, !((ap) & 0x2)))
>>>
>>> Similarly here.
>> ?
> 
> Can't just ignore AP bit 1 (or 0, if you're counting it that way).

with AP, the LSB is associated to EL0 rights. Similarly, given the
translation callback in use, is it something relevant as of now?

with APTable, the LSB bit is associated to EL0 as well
APTable[1] = 1, access at EL0 not permitted

In my case, can't I consider all accesses are priviledged?

Thanks

Eric
> 
> 
> thanks
> -- PMM
>