Re: [Xen-devel] [PATCH] amd-iommu: Fix Guest CR3 Table following c/s 3a7947b6901

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 03.04.19 at 20:08, <andrew.cooper3@xxxxxxxxxx> wrote:
> +static uint64_t dte_get_gcr3_table(const struct amd_iommu_dte *dte)
>  {
>      return ((dte->gcr3_trp_51_31 << 31) | (dte->gcr3_trp_30_15 << 15) |

I'm afraid there's another bug here: gcc, in its default mode at
least, doesn't promote bit field values to their declared types.
That is, the first of the shifts will truncate the high 20 bits, as it
gets carried out as a 32-bit operation. The expression result
then also is of type signed int, i.e. it'll get sign-extended to fill
the uint64_t.

> +static void dte_set_gcr3_table(struct amd_iommu_dte *dte, uint16_t dom_id,
> +                               uint64_t addr, uint8_t gv, uint8_t glx)

Perhaps better paddr_t and also bool for gv?

> +{
> +    /* I bit must be set when gcr3 is enabled */
> +    dte->i = 1;
> +
> +    dte->gcr3_trp_14_12 = MASK_EXTR(addr, 0x0000000000007000ull);
> +    dte->gcr3_trp_30_15 = MASK_EXTR(addr, 0x000000007fff8000ull);
> +    dte->gcr3_trp_51_31 = MASK_EXTR(addr, 0x000fffff80000000ull);

Like Paul I'm not in favor of such many digit constants, where one
always has to actually count digits to verify / understand the actual
values. But in the end it's up to the maintainers to judge.

> @@ -399,7 +423,7 @@ static int do_invalidate_dte(struct domain *d, 
> cmd_entry_t *cmd)
>      gdte = &dte_base[gbdf % (PAGE_SIZE / sizeof(struct amd_iommu_dte))];
>  
>      gdom_id = gdte->domain_id;
> -    gcr3_gfn = get_guest_cr3_from_dte(gdte);
> +    gcr3_gfn = dte_get_gcr3_table(gdte) >> PAGE_SHIFT;

PFN_DOWN() or paddr_to_pfn()?

> @@ -429,7 +453,7 @@ static int do_invalidate_dte(struct domain *d, 
> cmd_entry_t *cmd)
>      mdte = &dte_base[req_id];
>  
>      spin_lock_irqsave(&iommu->lock, flags);
> -    iommu_dte_set_guest_cr3(mdte, hdom_id, gcr3_mfn, gv, glx);
> +    dte_set_gcr3_table(mdte, hdom_id, gcr3_mfn << PAGE_SHIFT, gv, glx);

pfn_to_paddr()?

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel