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

[Peter Maydell]

On 7 March 2018 at 16:23, Auger Eric <address@hidden> wrote:
> Hi Peter,
>
> On 06/03/18 20:43, Peter Maydell wrote:
>> On 17 February 2018 at 18:46, Eric Auger <address@hidden> 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.

>>> +
>>> +#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).


thanks
-- PMM