Re: [Qemu-devel] [PATCH v7 10/20] hw/arm/smmuv3: Implement translate callback

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

Hi Peter,
On 09/10/17 19:45, Peter Maydell wrote:
> On 1 September 2017 at 18:21, Eric Auger <eric.auger@redhat.com> wrote:
>> This patch implements the IOMMU Memory Region translate()
>> callback. Most of the code relates to the translation
>> configuration decoding and check (STE, CD).
>>
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>> ---
>>  hw/arm/smmuv3-internal.h | 182 +++++++++++++++++++++++-
>>  hw/arm/smmuv3.c          | 351 ++++++++++++++++++++++++++++++++++++++++++++++-
>>  hw/arm/trace-events      |   9 ++
>>  3 files changed, 537 insertions(+), 5 deletions(-)
>>
>> diff --git a/hw/arm/smmuv3-internal.h b/hw/arm/smmuv3-internal.h
>> index e3e9828..f9f95ae 100644
>> --- a/hw/arm/smmuv3-internal.h
>> +++ b/hw/arm/smmuv3-internal.h
>> @@ -399,7 +399,185 @@ typedef enum evt_err {
>>      SMMU_EVT_E_PAGE_REQ     = 0x24,
>>  } SMMUEvtErr;
>>
>> -void smmuv3_record_event(SMMUV3State *s, hwaddr iova,
>> -                         uint32_t sid, bool is_write, SMMUEvtErr type);
>> +/*****************************
>> + * Configuration Data
>> + *****************************/
>> +
>> +typedef struct __smmu_data2  STEDesc; /* STE Level 1 Descriptor */
>> +typedef struct __smmu_data16 Ste;     /* Stream Table Entry(STE) */
>> +typedef struct __smmu_data2  CDDesc;  /* CD Level 1 Descriptor */
>> +typedef struct __smmu_data16 Cd;      /* Context Descriptor(CD) */
>> +
>> +/*****************************
>> + * STE fields
>> + *****************************/
>> +
>> +#define STE_VALID(x)   extract32((x)->word[0], 0, 1) /* 0 */
>> +#define STE_CONFIG(x)  extract32((x)->word[0], 1, 3)
>> +enum {
>> +    STE_CONFIG_NONE      = 0,
>> +    STE_CONFIG_BYPASS    = 4,       /* S1 Bypass    , S2 Bypass */
>> +    STE_CONFIG_S1        = 5,       /* S1 Translate , S2 Bypass */
>> +    STE_CONFIG_S2        = 6,       /* S1 Bypass    , S2 Translate */
>> +    STE_CONFIG_NESTED    = 7,       /* S1 Translate , S2 Translate */
>> +};
>> +#define STE_S1FMT(x)   extract32((x)->word[0], 4, 2)
>> +#define STE_S1CDMAX(x) extract32((x)->word[1], 27, 5)
>> +#define STE_EATS(x)    extract32((x)->word[2], 28, 2)
>> +#define STE_STRW(x)    extract32((x)->word[2], 30, 2)
>> +#define STE_S2VMID(x)  extract32((x)->word[4], 0, 16)
>> +#define STE_S2T0SZ(x)  extract32((x)->word[5], 0, 6)
>> +#define STE_S2SL0(x)   extract32((x)->word[5], 6, 2)
>> +#define STE_S2TG(x)    extract32((x)->word[5], 14, 2)
>> +#define STE_S2PS(x)    extract32((x)->word[5], 16, 3)
>> +#define STE_S2AA64(x)  extract32((x)->word[5], 19, 1)
>> +#define STE_S2HD(x)    extract32((x)->word[5], 24, 1)
>> +#define STE_S2HA(x)    extract32((x)->word[5], 25, 1)
>> +#define STE_S2S(x)     extract32((x)->word[5], 26, 1)
>> +#define STE_CTXPTR(x)                                           \
>> +    ({                                                          \
>> +        unsigned long addr;                                     \
>> +        addr = (uint64_t)extract32((x)->word[1], 0, 16) << 32;  \
>> +        addr |= (uint64_t)((x)->word[0] & 0xffffffc0);          \
>> +        addr;                                                   \
>> +    })
>> +
>> +#define STE_S2TTB(x)                                            \
>> +    ({                                                          \
>> +        unsigned long addr;                                     \
>> +        addr = (uint64_t)extract32((x)->word[7], 0, 16) << 32;  \
>> +        addr |= (uint64_t)((x)->word[6] & 0xfffffff0);          \
>> +        addr;                                                   \
>> +    })
>> +
>> +static inline int is_ste_bypass(Ste *ste)
> 
> Types which are acronyms are all-caps, so STE, CD.
> 
>> +{
>> +    return STE_CONFIG(ste) == STE_CONFIG_BYPASS;
>> +}
>> +
>> +static inline bool is_ste_stage1(Ste *ste)
>> +{
>> +    return STE_CONFIG(ste) == STE_CONFIG_S1;
>> +}
>> +
>> +static inline bool is_ste_stage2(Ste *ste)
>> +{
>> +    return STE_CONFIG(ste) == STE_CONFIG_S2;
>> +}
>> +
>> +/**
>> + * is_s2granule_valid - Check the stage 2 translation granule size
>> + * advertised in the STE matches any IDR5 supported value
>> + */
>> +static inline bool is_s2granule_valid(Ste *ste)
>> +{
>> +    int idr5_format = 0;
>> +
>> +    switch (STE_S2TG(ste)) {
>> +    case 0: /* 4kB */
>> +        idr5_format = 0x1;
>> +        break;
>> +    case 1: /* 64 kB */
>> +        idr5_format = 0x4;
>> +        break;
>> +    case 2: /* 16 kB */
>> +        idr5_format = 0x2;
>> +        break;
>> +    case 3: /* reserved */
>> +        break;
>> +    }
>> +    idr5_format &= SMMU_IDR5_GRAN;
>> +    return idr5_format;
>> +}
>> +
>> +static inline int oas2bits(int oas_field)
>> +{
>> +    switch (oas_field) {
>> +    case 0b011:
>> +        return 42;
>> +    case 0b100:
>> +        return 44;
>> +    default:
>> +        return 32 + (1 << oas_field);
>> +   }
>> +}
>> +
>> +static inline int pa_range(Ste *ste)
>> +{
>> +    int oas_field = MIN(STE_S2PS(ste), SMMU_IDR5_OAS);
>> +
>> +    if (!STE_S2AA64(ste)) {
>> +        return 40;
>> +    }
>> +
>> +    return oas2bits(oas_field);
>> +}
>> +
>> +#define MAX_PA(ste) ((1 << pa_range(ste)) - 1)
>> +
>> +/*****************************
>> + * CD fields
>> + *****************************/
>> +#define CD_VALID(x)   extract32((x)->word[0], 30, 1)
>> +#define CD_ASID(x)    extract32((x)->word[1], 16, 16)
>> +#define CD_TTB(x, sel)                                      \
>> +    ({                                                      \
>> +        uint64_t hi, lo;                                    \
>> +        hi = extract32((x)->word[(sel) * 2 + 3], 0, 16);    \
>> +        hi <<= 32;                                          \
>> +        lo = (x)->word[(sel) * 2 + 2] & ~0xf;               \
>> +        hi | lo;                                            \
>> +    })
>> +
>> +#define CD_TSZ(x, sel)   extract32((x)->word[0], (16 * (sel)) + 0, 6)
>> +#define CD_TG(x, sel)    extract32((x)->word[0], (16 * (sel)) + 6, 2)
>> +#define CD_EPD(x, sel)   extract32((x)->word[0], (16 * (sel)) + 14, 1)
>> +
>> +#define CD_T0SZ(x)    CD_TSZ((x), 0)
>> +#define CD_T1SZ(x)    CD_TSZ((x), 1)
>> +#define CD_TG0(x)     CD_TG((x), 0)
>> +#define CD_TG1(x)     CD_TG((x), 1)
>> +#define CD_EPD0(x)    CD_EPD((x), 0)
>> +#define CD_EPD1(x)    CD_EPD((x), 1)
>> +#define CD_IPS(x)     extract32((x)->word[1], 0, 3)
>> +#define CD_AARCH64(x) extract32((x)->word[1], 9, 1)
>> +#define CD_TTB0(x)    CD_TTB((x), 0)
>> +#define CD_TTB1(x)    CD_TTB((x), 1)
>> +
>> +#define CDM_VALID(x)    ((x)->word[0] & 0x1)
>> +
>> +static inline int is_cd_valid(SMMUV3State *s, Ste *ste, Cd *cd)
>> +{
>> +    return CD_VALID(cd);
>> +}
>> +
>> +/**
>> + * tg2granule - Decodes the CD translation granule size field according
>> + * to the TT in use
>> + * @bits: TG0/1 fiels
>> + * @tg1: if set, @bits belong to TG1, otherwise belong to TG0
>> + */
>> +static inline int tg2granule(int bits, bool tg1)
>> +{
>> +    switch (bits) {
>> +    case 1:
>> +        return tg1 ? 14 : 16;
>> +    case 2:
>> +        return tg1 ? 12 : 14;
>> +    case 3:
>> +        return tg1 ? 16 : 12;
>> +    default:
>> +        return 12;
>> +    }
>> +}
>> +
>> +#define L1STD_L2PTR(stm) ({                                 \
>> +            uint64_t hi, lo;                            \
>> +            hi = (stm)->word[1];                        \
>> +            lo = (stm)->word[0] & ~(uint64_t)0x1f;      \
>> +            hi << 32 | lo;                              \
>> +        })
>> +
>> +#define L1STD_SPAN(stm) (extract32((stm)->word[0], 0, 4))
>>
>>  #endif
>> diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
>> index 7470576..20fbce6 100644
>> --- a/hw/arm/smmuv3.c
>> +++ b/hw/arm/smmuv3.c
>> @@ -160,9 +160,9 @@ static void smmuv3_write_evtq(SMMUV3State *s, Evt *evt)
>>  /*
>>   * smmuv3_record_event - Record an event
>>   */
>> -void smmuv3_record_event(SMMUV3State *s, hwaddr iova,
>> -                         uint32_t sid, IOMMUAccessFlags perm,
>> -                         SMMUEvtErr type)
>> +static void smmuv3_record_event(SMMUV3State *s, hwaddr iova,
>> +                                uint32_t sid, IOMMUAccessFlags perm,
>> +                                SMMUEvtErr type)
>>  {
>>      Evt evt;
>>      bool rnw = perm & IOMMU_RO;
>> @@ -306,6 +306,348 @@ static inline void smmu_update_base_reg(SMMUV3State *s, uint64_t *base,
>>      *base = val & ~(SMMU_BASE_RA | 0x3fULL);
>>  }
>>
>> +/*
>> + * All SMMU data structures are little endian, and are aligned to 8 bytes
>> + * L1STE/STE/L1CD/CD, Queue entries in CMDQ/EVTQ/PRIQ
>> + */
>> +static inline int smmu_get_ste(SMMUV3State *s, hwaddr addr, Ste *buf)
>> +{
>> +    trace_smmuv3_get_ste(addr);
>> +    return dma_memory_read(&address_space_memory, addr, buf, sizeof(*buf));
> 
> Why dma_memory_read() here when we were using other memory access
> functions for things like TLB table walks earlier?
> 
> Incidentally, the spec requires us to perform memory accesses as
> at least 64-bit single-copy atomic (see 3.21.3) -- does this do that?
> (This gets important with SMP when the guest on another CPU might
> be updating the STE or page table entry at the same time as we're
> reading it...)

Among all your comments on v7, here is the one I am the least
comfortable with. I was not able to figure out whether
dma_memory_read(), which I use now for all the descriptor accesses
guarantees this 64b single copy atomicity.

Unrelated, I also did not change command descriptor field decoding (you
suggested to use registerfields.h). cmd struct is a a structure laid out
in guest RAM so I was not sure about how to use the register API for
this decoding.

With respect to the GPLv2 license issue, at the moment, I have not been
able to get an ACK from any Broadcom representative for transforming it
into "v2 or later". I will continue trying getting this approval though.

The IOMMU is not instantiated anymore using sysbus-fdt. it is
instantiated according to a machine option, still set to false by
default, given the performance overhead.

Otherwise I think I covered all your comments in v8.

As I mentioned in my cover letter I intend to submit separate patches
later on for
- vhost integration
- TLB emulation (as done on intel iommu),

as the code base already is huge and I am reluctant to add some other
features until the basic functionality has not stabilized.

Thanks

Eric
> 
>> +}
>> +
>> +/*
>> + * For now we only support CD with a single entry, 'ssid' is used to identify
>> + * otherwise
>> + */
>> +static inline int smmu_get_cd(SMMUV3State *s, Ste *ste, uint32_t ssid, Cd *buf)
>> +{
>> +    hwaddr addr = STE_CTXPTR(ste);
>> +
>> +    if (STE_S1CDMAX(ste) != 0) {
>> +        error_report("Multilevel Ctx Descriptor not supported yet");
>> +    }
>> +
>> +    trace_smmuv3_get_cd(addr);
>> +    return dma_memory_read(&address_space_memory, addr, buf, sizeof(*buf));
>> +}
>> +
>> +/**
>> + * is_ste_consistent - Check validity of STE
>> + * according to 6.2.1 Validity of STE
>> + * TODO: check the relevance of each check and compliance
>> + * with this spec chapter
> 
> Good idea :-)
> 
> thanks
> -- PMM
>