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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 14.07.15 at 01:39, <ravi.sahita@xxxxxxxxx> wrote:

> 
>>-----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).

I don't see why you shouldn't be able to factor this out without this
odd switch() nesting. Note that I didn't object to multiple sequential
switch() statements, only to their strange nesting...

Jan


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