Re: [PATCH V2 25/25] x86/vvtd: save and restore emulated VT-d

[Chao Gao <chao.gao@intel.com>]

On Fri, Aug 25, 2017 at 03:00:32AM -0600, Jan Beulich wrote:
>>>> On 25.08.17 at 08:35, <chao.gao@intel.com> wrote:
>> On Wed, Aug 23, 2017 at 01:19:41PM +0100, Roger Pau Monné wrote:
>>>On Wed, Aug 09, 2017 at 04:34:26PM -0400, Lan Tianyu wrote:
>>>> From: Chao Gao <chao.gao@intel.com>
>>>> 
>>>> Wrap some useful status in a new structure hvm_hw_vvtd, following
>>>> the customs of vlapic, vioapic and etc. Provide two save-restore
>>>> pairs 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>
>>>> ---
>>>> diff --git a/xen/include/public/arch-x86/hvm/save.h 
>> b/xen/include/public/arch-x86/hvm/save.h
>>>> index fd7bf3f..10536cb 100644
>>>> --- a/xen/include/public/arch-x86/hvm/save.h
>>>> +++ b/xen/include/public/arch-x86/hvm/save.h
>>>> @@ -639,10 +639,32 @@ struct hvm_msr {
>>>>  
>>>>  #define CPU_MSR_CODE  20
>>>>  
>>>> +struct hvm_hw_vvtd_regs {
>>>> +    uint8_t data[1024];
>>>> +};
>>>> +
>>>> +DECLARE_HVM_SAVE_TYPE(IOMMU_REGS, 21, struct hvm_hw_vvtd_regs);
>>>> +
>>>> +struct hvm_hw_vvtd
>>>> +{
>>>> +    /* VIOMMU_STATUS_XXX */
>>>> +    uint32_t status;
>>>> +    /* Fault Recording index */
>>>> +    uint32_t frcd_idx;
>>>> +    /* Is in Extended Interrupt Mode? */
>>>> +    uint32_t eim;
>>>> +    /* Max remapping entries in IRT */
>>>> +    uint32_t irt_max_entry;
>>>> +    /* Interrupt remapping table base gfn */
>>>> +    uint64_t irt;
>>>> +};
>>>> +
>>>> +DECLARE_HVM_SAVE_TYPE(IOMMU, 22, struct hvm_hw_vvtd);
>>>
>>>Why two separate structures? It should be the same structure.
>> 
>> Hi, Roger.
>> 
>> Thank you for your review. I agree with most of your comments on the
>> whole series. I will only reply to some points I think still need
>> discussion.
>> 
>> Here we use two separate structures for some field cannot be infered
>> from the struct hvm_hw_vvtd_regs. For example, the 'irt' is the gfn of
>> the base address Interrupt Remapping Table. The field is set through
>> 1. set the register DMAR_IRTE_REG in hvm_hw_vvtd_regs.
>> 2. send a command to vtd by writting another command register.
>> 
>> If the current base address is A, and guest wants to update the base
>> address to B and finish the first step. Unfortunately, saving and
>> restoring happen here. In this case, we need the struct hvm_hw_vvtd
>> to correctly restore some information.
>
>Hmm, the way I've understood Roger's question is why you
>don't combine the two structures into one, not whether one
>of the two can be omitted.

It seems likely that they can be combined. will give it a try.

Thanks
Chao

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