Re: [Xen-devel] [PATCH v4 11/15] x86/altp2m: define and implement alternate p2m HVMOP types.

["Sahita, Ravi" <ravi.sahita@xxxxxxxxx>]

>-----Original Message-----
>From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
>Sent: Monday, July 13, 2015 12:26 AM
>To: Sahita, Ravi
>Cc: Andrew Cooper; Wei Liu; George Dunlap; Ian Jackson; White, Edmund H;
>xen-devel@xxxxxxxxxxxxx; tlengyel@xxxxxxxxxxx; Daniel De Graaf; Tim
>Deegan
>Subject: RE: [PATCH v4 11/15] x86/altp2m: define and implement alternate
>p2m HVMOP types.
>
>>>> On 11.07.15 at 00:03, <ravi.sahita@xxxxxxxxx> wrote:
>>> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
>>>Sent: Friday, July 10, 2015 3:01 AM
>>>>>> On 10.07.15 at 02:52, <edmund.h.white@xxxxxxxxx> wrote:
>>>> +        default:
>>>> +            return -ENOSYS;
>>>> +
>>>> +            break;
>>>
>>>Bogus (unreachable) break.
>>
>> Wanted to keep this so that if someone removes the error code then
>> they don't cause an invalid fall through.
>> But ok with removing it if you think so.
>
>We don't (intentionally) do this anywhere else, so it should be removed.

Done

>
>>>> +                if ( !(d ? d : current->domain)->arch.altp2m_active
>>>> + )
>>>
>>>This is bogus: d is NULL if and only if altp2m_vcpu_enable_notify,
>>>i.e. I
>> don't
>>>see why you can't just use current->domain inside that case (and you
>>>really do). That would then also eliminate the need for this redundant
>>>and obfuscating switch() nesting you use.
>>>
>>
>> We need to check if the target domain is in altp2m mode for all the
>> following sub-ops.
>> If we removed this check, we would need to check for target domain
>> being in altp2m for all the following cases.
>> Andrew wanted to refactor and pull common code up, and that's what
>> this is one case of for hvm_op.
>
>I'd be fine with such refactoring if it didn't result in nested switch()-es 
>using
>the same control expression.
>

Agree that removing the current->domain test will allow us to remove the 
switch() nesting, however that will also require replicating the common code 
blocks - I'm ok with making this change just want an ok from Andrew on this one 
before we make this change to avoid thrashing on this one (since he was the one 
asking for the refactor).


>>>> +
>>>> +struct xen_hvm_altp2m_set_mem_access {
>>>> +    /* view */
>>>> +    uint16_t view;
>>>> +    /* Memory type */
>>>> +    uint16_t hvmmem_access; /* xenmem_access_t */
>>>> +    uint8_t pad[4];
>>>> +    /* gfn */
>>>> +    uint64_t gfn;
>>>> +};
>>>> +typedef struct xen_hvm_altp2m_set_mem_access
>>>> xen_hvm_altp2m_set_mem_access_t;
>>>> +DEFINE_XEN_GUEST_HANDLE(xen_hvm_altp2m_set_mem_access_t);
>>>> +
>>>> +struct xen_hvm_altp2m_change_gfn {
>>>> +    /* view */
>>>> +    uint16_t view;
>>>> +    uint8_t pad[6];
>>>> +    /* old gfn */
>>>> +    uint64_t old_gfn;
>>>> +    /* new gfn, INVALID_GFN (~0UL) means revert */
>>>> +    uint64_t new_gfn;
>>>> +};
>>>> +typedef struct xen_hvm_altp2m_change_gfn
>>>xen_hvm_altp2m_change_gfn_t;
>>>> +DEFINE_XEN_GUEST_HANDLE(xen_hvm_altp2m_change_gfn_t);
>>>> +
>>>> +struct xen_hvm_altp2m_op {
>>>> +    uint32_t cmd;
>>>> +/* Get/set the altp2m state for a domain */
>>>> +#define HVMOP_altp2m_get_domain_state     1
>>>> +#define HVMOP_altp2m_set_domain_state     2
>>>> +/* Set the current VCPU to receive altp2m event notifications */
>>>> +#define HVMOP_altp2m_vcpu_enable_notify   3
>>>> +/* Create a new view */
>>>> +#define HVMOP_altp2m_create_p2m           4
>>>> +/* Destroy a view */
>>>> +#define HVMOP_altp2m_destroy_p2m          5
>>>> +/* Switch view for an entire domain */
>>>> +#define HVMOP_altp2m_switch_p2m           6
>>>> +/* Notify that a page of memory is to have specific access types */
>>>> +#define HVMOP_altp2m_set_mem_access       7
>>>> +/* Change a p2m entry to have a different gfn->mfn mapping */
>>>> +#define HVMOP_altp2m_change_gfn           8
>>>> +    domid_t domain;
>>>> +    uint8_t pad[2];
>>>
>>>While you added padding fields as asked for, you still don't verify
>>>them to
>> be
>>>zero on input.
>>
>> Specifically what were you thinking we need to do here - also would be
>> good if you can explain what was the underlying concern? (thanks)
>
>I'm pretty sure I said so before - future extensibility. I.e. a means to make 
>use
>of the now unused (padding) fields, which can only be done if the fields are
>being checked to be zero while unused.
>
>Jan

Fixed this

Ravi


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