Re: [PATCH v2 1/9] x86/shadow: replace sh_reset_l3_up_pointers()

[George Dunlap <george.dunlap@xxxxxxxxx>]

 

On Mon, Jan 23, 2023 at 8:41 AM Jan Beulich <jbeulich@xxxxxxxx> wrote:

On 20.01.2023 18:02, George Dunlap wrote:
> On Wed, Jan 11, 2023 at 1:52 PM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>
>> Rather than doing a separate hash walk (and then even using the vCPU
>> variant, which is to go away), do the up-pointer-clearing right in
>> sh_unpin(), as an alternative to the (now further limited) enlisting on
>> a "free floating" list fragment. This utilizes the fact that such list
>> fragments are traversed only for multi-page shadows (in shadow_free()).
>> Furthermore sh_terminate_list() is a safe guard only anyway, which isn't
>> in use in the common case (it actually does anything only for BIGMEM
>> configurations).
>
> One thing that seems strange about this patch is that you're essentially
> adding a field to the domain shadow struct in lieu of adding another
> another argument to sh_unpin() (unless the bit is referenced elsewhere in
> subsequent patches, which I haven't reviewed, in part because about half of
> them don't apply cleanly to the current tree).

Well, to me adding another parameter to sh_unpin() would have looked odd;
the new field looks slightly cleaner to me. But changing that is merely a
matter of taste, so if you and e.g. Andrew think that approach was better,
I could switch to that. And no, I don't foresee further uses of the field.

You're about to call sh_unpin(), and you want to tell that function to change its behavior.  What's so odd about adding an argument to the function to indicate the behavior?  Instead you're adding a bit of global state which is carried around 100% of the time, even when that function isn't being called.  That's not what people normally expect; it makes the code harder to reason about.

It would certainly be ugly to have to add "false" to every other instance of sh_unpin; but the normal way you get around that is to redefine sh_unpin() as a wrapper which calls the other function with the 'false' argument set.

You asked me to review this for a second opinion on the safety of clearing the up-pointer this way, not because you need an ack; so I don't really want to block the patch for non-functional reasons.  But I think this is one of the "death by a thousand cuts" that makes the shadow code more fragile and difficult for new people to approach and understand.

 

Re the original question: I've stared at the code for a bit now, and I can't see anything obviously wrong or dangerous about it.

But it does make me ask, why do we need the "unpinning_l3" pseudo-argument at all?  Is there any reason not to unconditionally zero out sp->up when we find a head_type of SH_type_l3_64_shadow?  As far as I can tell, sp->list doesn't require any special state.  Why do we make the effort to leave it alone when we're not unpinning all l3s?

In fact, is there a way to unpin an l3 shadow *other* than when we're unpinning all l3's?  If so, then this patch, as written, is broken -- the original code clears the up-pointer for *all* L3_64 shadows, regardless of whether they're on the pinned list; the new patch will only clear the ones on the pinned list.  But unconditionally clearing sp->up could actually fix that.

Thoughts?

As to half of the patches not applying: Some where already applied out of
order, and others therefore need re-basing slightly. Till now I saw no
reason to re-send the remaining patches just for that.

Sorry if that sounded like complaining; I was only being preemptively defensive against the potential accusation that the answer would have been obvious if I'd just continued reviewing the series. :-). (And indeed if the whole series had applied I would have checked that the final result didn't have any other references to it.)

 -George