Re: [PATCH v4 17/28] x86/vvtd: save and restore emulated VT-d

From: Chao Gao <chao.gao@intel.com>
To: "Roger Pau Monné" <roger.pau@citrix.com>
Cc: Lan Tianyu <tianyu.lan@intel.com>,
	Kevin Tian <kevin.tian@intel.com>,
	Stefano Stabellini <sstabellini@kernel.org>,
	Wei Liu <wei.liu2@citrix.com>,
	George Dunlap <george.dunlap@eu.citrix.com>,
	Ian Jackson <ian.jackson@eu.citrix.com>, Tim Deegan <tim@xen.org>,
	xen-devel@lists.xen.org, Jan Beulich <jbeulich@suse.com>,
	Andrew Cooper <andrew.cooper3@citrix.com>
Subject: Re: [PATCH v4 17/28] x86/vvtd: save and restore emulated VT-d
Date: Fri, 23 Feb 2018 13:22:23 +0800	[thread overview]
Message-ID: <20180223052223.GB162485@skl-4s-chao.sh.intel.com> (raw)
In-Reply-To: <20180212144912.jywfmqlowmkrsmeq@MacBook-Pro-de-Roger.local>

On Mon, Feb 12, 2018 at 02:49:12PM +0000, Roger Pau Monné wrote:
>On Fri, Nov 17, 2017 at 02:22:24PM +0800, Chao Gao wrote:
>> Provide a save-restore pair to save/restore registers and non-register
>> status.
>> 
>> Signed-off-by: Chao Gao <chao.gao@intel.com>
>> Signed-off-by: Lan Tianyu <tianyu.lan@intel.com>
>> ---
>> v3:
>>  - use one entry to save both vvtd registers and other intermediate
>>  state
>> ---
>>  xen/drivers/passthrough/vtd/vvtd.c     | 57 +++++++++++++++++++++++-----------
>>  xen/include/public/arch-x86/hvm/save.h | 18 ++++++++++-
>>  2 files changed, 56 insertions(+), 19 deletions(-)
>> 
>> diff --git a/xen/drivers/passthrough/vtd/vvtd.c b/xen/drivers/passthrough/vtd/vvtd.c
>> index 81170ec..f6bde69 100644
>> --- a/xen/drivers/passthrough/vtd/vvtd.c
>> +++ b/xen/drivers/passthrough/vtd/vvtd.c
>> @@ -27,8 +27,10 @@
>>  #include <asm/event.h>
>>  #include <asm/io_apic.h>
>>  #include <asm/hvm/domain.h>
>> +#include <asm/hvm/save.h>
>>  #include <asm/hvm/support.h>
>>  #include <asm/p2m.h>
>> +#include <public/hvm/save.h>
>>  
>>  #include "iommu.h"
>>  #include "vtd.h"
>> @@ -38,20 +40,6 @@
>>  
>>  #define VVTD_FRCD_NUM   1ULL
>>  #define VVTD_FRCD_START (DMAR_IRTA_REG + 8)
>> -#define VVTD_FRCD_END   (VVTD_FRCD_START + VVTD_FRCD_NUM * 16)
>> -#define VVTD_MAX_OFFSET VVTD_FRCD_END
>> -
>> -struct hvm_hw_vvtd {
>> -    bool eim_enabled;
>> -    bool intremap_enabled;
>> -    uint32_t fault_index;
>> -
>> -    /* Interrupt remapping table base gfn and the max of entries */
>> -    uint16_t irt_max_entry;
>> -    gfn_t irt;
>
>You are changing gfn_t to uint64_t, is gfn_t not working with the
>migration stream?

In xen/include/public/save.h, there is a comment around line 32:
 * Structures in this header *must* have the same layout in 32bit 
 * and 64bit environments: this means that all fields must be explicitly 
 * sized types and aligned to their sizes, and the structs must be 
 * a multiple of eight bytes long.

That's why I change bool to uint32_t and gfn_t to uint64_t.

>
>Also I think this duplication of fields (having all registers in
>'regs' and some cached in miscellaneous top level fields is not a good
>approach.

Yes. I think intremap_enabled can be removed. Others (i.e.
eim_enable, irt_max_entry, irt) cannot be removed. Because
guest may update IRTA when interrupt remapping is enabled.
Here means guest wants to use TABLE B to replace current TABLE A.
To finish this replacing, guest should program table B's base address,
the number of entries and eim is enabled or not to IRTA and then set
GCMD with SIRTP set. If saving and restoring happen between above two
steps, we can recover TABLE A's base address and other information.

>
>> -
>> -    uint32_t regs[VVTD_MAX_OFFSET/sizeof(uint32_t)];
>> -};
>>  
>>  struct vvtd {
>>      /* Base address of remapping hardware register-set */
>> @@ -776,7 +764,7 @@ static void write_gcmd_sirtp(struct vvtd *vvtd, uint32_t val)
>>      if ( vvtd->hw.intremap_enabled )
>>          vvtd_info("Update Interrupt Remapping Table when active\n");
>>  
>> -    if ( gfn_x(vvtd->hw.irt) != PFN_DOWN(DMA_IRTA_ADDR(irta)) ||
>> +    if ( vvtd->hw.irt != PFN_DOWN(DMA_IRTA_ADDR(irta)) ||
>>           vvtd->hw.irt_max_entry != DMA_IRTA_SIZE(irta) )
>>      {
>>          if ( vvtd->irt_base )
>> @@ -786,14 +774,14 @@ static void write_gcmd_sirtp(struct vvtd *vvtd, uint32_t val)
>>                                       sizeof(struct iremap_entry)));
>>              vvtd->irt_base = NULL;
>>          }
>> -        vvtd->hw.irt = _gfn(PFN_DOWN(DMA_IRTA_ADDR(irta)));
>> +        vvtd->hw.irt = PFN_DOWN(DMA_IRTA_ADDR(irta));
>>          vvtd->hw.irt_max_entry = DMA_IRTA_SIZE(irta);
>>          vvtd->hw.eim_enabled = !!(irta & IRTA_EIME);
>>          vvtd_info("Update IR info (addr=%lx eim=%d size=%d)\n",
>> -                  gfn_x(vvtd->hw.irt), vvtd->hw.eim_enabled,
>> +                  vvtd->hw.irt, vvtd->hw.eim_enabled,
>>                    vvtd->hw.irt_max_entry);
>>  
>> -        vvtd->irt_base = map_guest_pages(vvtd->domain, gfn_x(vvtd->hw.irt),
>> +        vvtd->irt_base = map_guest_pages(vvtd->domain, vvtd->hw.irt,
>>                                           PFN_UP(vvtd->hw.irt_max_entry *
>>                                                  sizeof(struct iremap_entry)));
>>      }
>> @@ -1138,6 +1126,39 @@ static bool vvtd_is_remapping(const struct domain *d,
>>      return !irq_remapping_request_index(irq, &idx);
>>  }
>>  
>> +static int vvtd_load(struct domain *d, hvm_domain_context_t *h)
>> +{
>> +    struct vvtd *vvtd = domain_vvtd(d);
>> +    uint64_t iqa;
>> +
>> +    if ( !vvtd )
>> +        return -ENODEV;
>> +
>> +    if ( hvm_load_entry(VVTD, h, &vvtd->hw) )
>> +        return -EINVAL;
>> +
>> +    iqa = vvtd_get_reg_quad(vvtd, DMAR_IQA_REG);
>> +    vvtd->irt_base = map_guest_pages(vvtd->domain, vvtd->hw.irt,
>> +                                     PFN_UP(vvtd->hw.irt_max_entry *
>> +                                            sizeof(struct iremap_entry)));
>> +    vvtd->inv_queue_base = map_guest_pages(vvtd->domain,
>> +                                           PFN_DOWN(DMA_IQA_ADDR(iqa)),
>> +                                           1 << DMA_IQA_QS(iqa));
>
>Why are you unconditionally mapping those pages? Shouldn't you check
>that the relevant features are enabled?
>
>Both could be 0 or simply point to garbage.

Will do some checks.

>
>> +    return 0;
>> +}
>> +
>> +static int vvtd_save(struct domain *d, hvm_domain_context_t *h)
>> +{
>> +    struct vvtd *vvtd = domain_vvtd(d);
>> +
>> +    if ( !vvtd )
>> +        return 0;
>> +
>> +    return hvm_save_entry(VVTD, 0, h, &vvtd->hw);
>> +}
>> +
>> +HVM_REGISTER_SAVE_RESTORE(VVTD, vvtd_save, vvtd_load, 1, HVMSR_PER_DOM);
>> +
>>  static void vvtd_reset(struct vvtd *vvtd)
>>  {
>>      uint64_t cap = cap_set_num_fault_regs(VVTD_FRCD_NUM)
>> diff --git a/xen/include/public/arch-x86/hvm/save.h b/xen/include/public/arch-x86/hvm/save.h
>> index fd7bf3f..24a513b 100644
>> --- a/xen/include/public/arch-x86/hvm/save.h
>> +++ b/xen/include/public/arch-x86/hvm/save.h
>> @@ -639,10 +639,26 @@ struct hvm_msr {
>>  
>>  #define CPU_MSR_CODE  20
>>  
>> +#define VVTD_MAX_OFFSET 0xd0
>
>You used to have some kind of formula to calculate VVTD_MAX_OFFSET,
>yet here the value is just hardcoded. Any reason for this?

The formula uses DMAR_IRTA_REG, which is defined in xen/drivers/passthrough/vtd/iommu.h.
It cannot be included by this file.

>
>> +struct hvm_hw_vvtd
>> +{
>> +    uint32_t eim_enabled : 1,
>> +             intremap_enabled : 1;
>> +    uint32_t fault_index;
>> +
>> +    /* Interrupt remapping table base gfn and the max of entries */
>> +    uint32_t irt_max_entry;
>> +    uint64_t irt;
>> +
>> +    uint32_t regs[VVTD_MAX_OFFSET/sizeof(uint32_t)];
>> +};
>> +
>> +DECLARE_HVM_SAVE_TYPE(VVTD, 21, struct hvm_hw_vvtd);
>
>Adding new fields to this struct in a migration compatible way is
>going to be a PITA, but there's no easy solution to this I'm afraid...

What do you mean by "migration compatible"? Do you mean migrating a hvm
guest with viommu between different Xen versions? Could it be solved by
leaving some padding fields here?

Thanks
Chao

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