Re: [PATCH 02/12] hw/i386/intel_iommu: vtd_slpte_nonzero_rsvd(): reduce magic numbers

[Vladimir Sementsov-Ogievskiy]

On 26.09.23 13:37, Peter Maydell wrote:
> On Mon, 25 Sept 2023 at 20:41, Vladimir Sementsov-Ogievskiy
> <vsementsov@yandex-team.ru> wrote:
> > Add a constant and clear assertion. The assertion also tells Coverity
> > that we are not going to overflow the array.
> >
> > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> > ---
> >   hw/i386/intel_iommu.c | 11 ++++++++---
> >   1 file changed, 8 insertions(+), 3 deletions(-)
> >
> > diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> > index c0ce896668..2233dbe13a 100644
> > --- a/hw/i386/intel_iommu.c
> > +++ b/hw/i386/intel_iommu.c
> > @@ -1028,12 +1028,17 @@ static dma_addr_t 
> > vtd_get_iova_pgtbl_base(IntelIOMMUState *s,
> >    *     vtd_spte_rsvd 4k pages
> >    *     vtd_spte_rsvd_large large pages
> >    */
> > -static uint64_t vtd_spte_rsvd[5];
> > -static uint64_t vtd_spte_rsvd_large[5];
> > +#define VTD_SPTE_RSVD_LEN 5
> > +static uint64_t vtd_spte_rsvd[VTD_SPTE_RSVD_LEN];
> > +static uint64_t vtd_spte_rsvd_large[VTD_SPTE_RSVD_LEN];
> >
> >   static bool vtd_slpte_nonzero_rsvd(uint64_t slpte, uint32_t level)
> >   {
> > -    uint64_t rsvd_mask = vtd_spte_rsvd[level];
> > +    uint64_t rsvd_mask;
> > +
> > +    assert(level < VTD_SPTE_RSVD_LEN);
> > +
> > +    rsvd_mask = vtd_spte_rsvd[level];
>
>
> Looking at the code it is not clear to me why this assertion is
> valid. It looks like we are picking up fields from guest-set
> configuration (probably in-memory data structures). So we can't
> assert() here -- we need to do whatever the real hardware does
> if these fields are set to an incorrect value, or at least something
> sensible that doesn't crash QEMU.

Finally, seems that assertion is valid. We do check the guest-set configuration:

1. in vtd_decide_config(), we check that s->aw_bits is exactly one of 
VTD_HOST_AW_39BIT or VTD_HOST_AW_48BIT.

2. in vtd_init(), in s->cap we set VTD_CAP_SAGAW_39bit (bit 1) and may be 
VTD_CAP_SAGAW_48bit (bit 2),  but never bit 3 (which would allow 5-level 
page-table) or any other bit (i.e. bits 0 and 4 which are reserved).

3. then, as I could follow, both context entry and pasid entry should go through 
vtd_is_level_supported(), which checks that level is allowed in s->cap.

So in the code we should work only with levels 3 and 4.



--
Best regards,
Vladimir