[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [RFC PATCH 05/16] hw/arm/smmuv3: Add page table walk for stage-2
From: |
Mostafa Saleh |
Subject: |
Re: [RFC PATCH 05/16] hw/arm/smmuv3: Add page table walk for stage-2 |
Date: |
Thu, 16 Feb 2023 13:09:33 +0000 |
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.
> > + 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 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.
> > @@ -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
[RFC PATCH 05/16] hw/arm/smmuv3: Add page table walk for stage-2, Mostafa Saleh, 2023/02/05
[RFC PATCH 06/16] hw/arm/smmuv3: Parse STE config for stage-2, Mostafa Saleh, 2023/02/05
[RFC PATCH 07/16] hw/arm/smmuv3: Check validity of stage-2 page table, Mostafa Saleh, 2023/02/05
[RFC PATCH 08/16] hw/arm/smmuv3: Support S2AFFD, Mostafa Saleh, 2023/02/05